Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stdenv/linux/default.nix: add gcc rebuild during bootstrap #209063

Closed
wants to merge 1 commit into from

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Jan 4, 2023

Description of changes

A bit of background how set of changes came to be with more words: https://trofi.github.io/posts/275-nixpkgs-bootstrap-deep-dive.html

before the change our rebuild chain was roughly the followin:

  • bootstrapTools install
  • binutils built by bootstrapTools
  • glibc built by bootstrapTools
  • gcc built by bootstrapTools

As a result glibc contained code generated by bootstrapTools's
gcc. And (what worse) copied libgcc_s.so.1 as is from it.

Such libgcc_s.so.1 was not compatible with nixpkgs gcc at
least on aarch64 where newer libgcc is expected to have new symbols.
As a result linking failed as:

ld: /build/test.o: in function `foo(int)':
test.cpp:(.text+0x2c): undefined reference to `__aarch64_ldadd4_acq_rel'
collect2: error: ld returned 1 exit status

The change rejigs rebuild sequence as:

  • bootstrapTools install
  • binutils built by bootstrapTools
  • gcc built by bootstrapTools
  • glibc built by gcc
  • gcc built by gcc

As a result glibc gets built by fresher gcc and embeds a copy
of libgcc_s.so.1.

To make it work as is I had to use a --sysroot= to evade
accidental pull of system headers embedded by gcc.

While at it decreased rebuild counts for bash and xz. These are
simple tools we can use as is built by bootstrapTools until we
build final stage.

List of packages with more than one build before the change:

$ nix-store --query --graph $(nix-instantiate -A stdenv) |
    grep -P " -> " | awk '{print $3}' | sort -u | sed 's/"[0-9a-z]\{32\}-/"/g' |
    sort | uniq -c | sort -n | awk '$1 > 1'

  2 "autoconf-2.71.drv"
  2 "automake-1.16.5.drv"
  2 "bootstrap-stage1-stdenv-linux.drv"
  2 "bootstrap-stage2-stdenv-linux.drv"
  2 "bootstrap-stage4-stdenv-linux.drv"
  2 "bzip2-1.0.8.drv"
  2 "file-5.43.drv"
  2 "help2man-1.49.2.drv"
  2 "hook.drv"
  2 "libtool-2.4.7.drv"
  2 "patchelf-0.15.0.drv"
  2 "perl5.36.0-gettext-1.07.drv"
  3 "binutils-2.39.drv"
  3 "binutils-wrapper-2.39.drv"
  3 "expand-response-params.drv"
  3 "gettext-0.21.drv"
  3 "libxcrypt-4.4.33.drv"
  3 "perl-5.36.0.drv"
  3 "texinfo-6.8.drv"
  3 "zlib-1.2.13.drv"
  4 "bash-5.1-p16.drv"
  4 "xz-5.2.9.drv"

List of packages with more than one build after the change:

$ nix-store --query --graph $(nix-instantiate -A stdenv) |
    grep -P " -> " | awk '{print $3}' | sort -u | sed 's/"[0-9a-z]\{32\}-/"/g' |
    sort | uniq -c | sort -n | awk '$1 > 1'

  2 "autoconf-2.71.drv"
  2 "automake-1.16.5.drv"
  2 "bash-5.2-p15.drv"
  2 "bootstrap-stage1-stdenv-linux.drv"
  2 "bootstrap-stage2-stdenv-linux.drv"
  2 "bootstrap-stage3-stdenv-linux.drv"
  2 "bootstrap-stage5-stdenv-linux.drv"
  2 "bzip2-1.0.8.drv"
  2 "file-5.43.drv"
  2 "gcc-11.3.0.drv"
  2 "gmp-with-cxx-6.2.1.drv"
  2 "help2man-1.49.2.drv"
  2 "hook.drv"
  2 "libtool-2.4.7.drv"
  2 "mpfr-4.1.1.drv"
  2 "patchelf-0.15.0.drv"
  2 "perl5.36.0-gettext-1.07.drv"
  2 "texinfo-6.8.drv"
  2 "which-2.21.drv"
  2 "xz-5.4.0.drv"
  3 "perl-5.36.0.drv"
  4 "binutils-2.39.drv"
  4 "binutils-wrapper-2.39.drv"
  4 "expand-response-params.drv"
  4 "gettext-0.21.drv"
  4 "libxcrypt-4.4.33.drv"
  4 "zlib-1.2.13.drv"
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

pkgs/stdenv/linux/default.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@K900 K900 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please.

@trofi
Copy link
Contributor Author

trofi commented Jan 4, 2023

Sorry, last minute "fix" slipped in and there is a one unwanted reference to bootstrap tools that fails the stdenv build. The fix is one-liner and will not rebuild too much if you already tried the branch:

--- a/pkgs/stdenv/linux/default.nix
+++ b/pkgs/stdenv/linux/default.nix
@@ -459,7 +459,8 @@ in
       # other purposes (binutils and top-level pkgs) too.
       inherit (prevStage)
           bash gettext gnum4 bison
-          perl texinfo which xz
+          perl texinfo which
+          # xz - rebuild to pull in rebuilt version into final stdenv
           zlib linuxHeaders libidn2 libunistring;

       ${localSystem.libc} = getLibc prevStage;

I'll push it in a few minutes along with comment updates.

@trofi trofi force-pushed the extra-gcc-build-during-bootstrap branch from 4965bd8 to 6428d11 Compare January 4, 2023 18:41
@trofi
Copy link
Contributor Author

trofi commented Jan 4, 2023

I'll push it in a few minutes along with comment updates.

Pushed the update. Was able to build stdenv on x86_64 and a bunch of c++ packages on top of it.

Should be ready to test.

@K900
Copy link
Contributor

K900 commented Jan 4, 2023

Currently attempting to bootstrap my entire server from this branch, will see if it builds.

@tpwrules
Copy link
Contributor

tpwrules commented Jan 4, 2023

I'm rebuilding my entire aarch64 test VM with a pretty standard Xfce + Firefox on this branch. Then I will try again with GCC 11.

@K900
Copy link
Contributor

K900 commented Jan 4, 2023

I've successfully built about two thirds of my server config with this branch, including pretty much every compiler under the sun (Clang, GHC, gfortran, rustc, Ruby, Python, Erlang, Elixir) in the chain. I'll leave it running to build the server config and also my desktop config overnight and see how that goes, but it's very likely this is good to go on x86_64-linux.

@K900
Copy link
Contributor

K900 commented Jan 5, 2023

Aaaand the server config is built, with one unrelated failure (#209118). Time for the desktop, and time for me to go get some sleep.

@K900
Copy link
Contributor

K900 commented Jan 5, 2023

The desktop build ended up running out of disk space, but it got almost all the way there. I think this should really be good to go. Has anyone tested aarch64 with GCC 11 yet?

@Mindavi
Copy link
Contributor

Mindavi commented Jan 5, 2023

Does this still not violate the stance that none of the files in bootstrap or generated by bootstrap tools are not used directly?

@trofi trofi force-pushed the extra-gcc-build-during-bootstrap branch from 3c51469 to 1210fc2 Compare January 5, 2023 12:15
@trofi
Copy link
Contributor Author

trofi commented Jan 5, 2023

Got another snag: vte (uses clangStdenv) failed for inability to dins libstdc++ librray. It's a side-effect of --sysroot= hack. Added a safe workaround for it to provide cross-looking gcc.lib paths with:

--- a/pkgs/development/compilers/gcc/11/default.nix
+++ b/pkgs/development/compilers/gcc/11/default.nix
@@ -221,6 +221,7 @@ stdenv.mkDerivation ({
   };

   targetConfig = if targetPlatform != hostPlatform then targetPlatform.config else null;
+  targetPlatformConfig = targetPlatform.config;

   buildFlags = optional
     (targetPlatform == hostPlatform && hostPlatform == buildPlatform)
diff --git a/pkgs/development/compilers/gcc/builder.sh b/pkgs/development/compilers/gcc/builder.sh
index 113bd83ea53..71a997a7df9 100644
--- a/pkgs/development/compilers/gcc/builder.sh
+++ b/pkgs/development/compilers/gcc/builder.sh
@@ -203,6 +203,17 @@ preInstall() {
         ln -s lib "$out/${targetConfig}/lib32"
         ln -s lib "${!outputLib}/${targetConfig}/lib32"
     fi
+
+    # cc-wrappers uses --sysroot=/nix/dir/does/not/exist as a way to
+    # drop default sysheaders search path. Unfortunately that switches
+    # clang++ into searching libraries in gcc in cross-compiler paths:
+    #   from ${!outputLib}/lib (native)
+    #   to ${!outputLib}/${targetPlatformConfig}/lib
+    # We create the symlink to make both native and cross paths
+    # available even if the toolchain is not the cross-compiler.
+    if [ ! -e ${!outputLib}/${targetPlatformConfig} ] ; then
+        ln -s . ${!outputLib}/${targetPlatformConfig}
+    fi
 }

Applied to all gcc versions. It fixes vte and other clang-based builds. Did not seem to break pkgsLLVM or cross-builds either.

@trofi trofi force-pushed the extra-gcc-build-during-bootstrap branch from 68f9e71 to 3e48d0b Compare January 5, 2023 12:19
@trofi
Copy link
Contributor Author

trofi commented Jan 10, 2023

Restores excessive rebuilds of xz, texinfo and friends to make main commit leaner. Will reintroduce it separately this evening.

@trofi
Copy link
Contributor Author

trofi commented Jan 10, 2023

Restores excessive rebuilds of xz, texinfo and friends to make main commit leaner. Will reintroduce it separately this evening.

Proposed 2 of 3 perl builds removal separately in #210118

@trofi
Copy link
Contributor Author

trofi commented Jan 11, 2023

Proposed one texinfo removal from binutils as #210132

@trofi
Copy link
Contributor Author

trofi commented Jan 11, 2023

Filed upstream bug for poor build parallelism in gettext: https://savannah.gnu.org/bugs/index.php?63641

@trofi
Copy link
Contributor Author

trofi commented Jan 14, 2023

No functional change. No need to retest.

Rebased against current staging. This left us with a single gcc stage addition change (if we ignore #210741)

While at it added missing minor rename:

--- a/pkgs/stdenv/linux/default.nix
+++ b/pkgs/stdenv/linux/default.nix
@@ -415,7 +415,7 @@ in
       gcc-unwrapped =
         let makeStaticLibrariesAndMark = pkg:
               lib.makeOverridable (pkg.override { stdenv = self.makeStaticLibraries self.stdenv; })
-                .overrideAttrs (a: { pname = "${a.pname}-stage3"; });
+                .overrideAttrs (a: { pname = "${a.pname}-stage4"; });
         in super.gcc-unwrapped.override {
         # Link GCC statically against GMP etc.  This makes sense because
         # these builds of the libraries are only used by GCC, so it
@@ -466,7 +466,7 @@ in
       # force gmp to rebuild so we have the option of dynamically linking
       # libgmp without creating a reference path from:
       #   stage5.gcc -> stage4.coreutils -> stage3.glibc -> bootstrap
-      gmp = lib.makeOverridable (super.gmp.override { stdenv = self.stdenv; }).overrideAttrs (a: { pname = "${a.pname}-stage4"; });
+      gmp = lib.makeOverridable (super.gmp.override { stdenv = self.stdenv; }).overrideAttrs (a: { pname = "${a.pname}-stage5"; });

       # To allow users' overrides inhibit dependencies too heavy for
       # bootstrap, like guile: https://github.com/NixOS/nixpkgs/issues/181188

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 14, 2023
@wegank
Copy link
Member

wegank commented Jan 14, 2023

@trofi I guess this PR is now superseded by #210741?

@trofi
Copy link
Contributor Author

trofi commented Jan 15, 2023

@trofi I guess this PR is now superseded by #210741?

No, this PR includes that one and adds a gcc rebuild stage step.

Before the change our rebuild chain was the followin:

- `bootstrapTools` install
- `binutils` built by `bootstrapTools`
- `glibc` built by `bootstrapTools`
- `gcc` built by `bootstrapTools`

As a result `glibc` contained code generated by `bootstrapTools`'s
`gcc`. And (what worse) copied `libgcc_s.so.1` as is from
`bootstrapTools`'s `gcc`.

Such `libgcc_s.so.1` was not compatible with `nixpkgs` `gcc` at
least on `aarch64` where newer `libgcc` is expected to have new symbols.
As a result linking failed as:

    ld: /build/test.o: in function `foo(int)':
    test.cpp:(.text+0x2c): undefined reference to `__aarch64_ldadd4_acq_rel'
    collect2: error: ld returned 1 exit status

The change rejigs rebuild sequence as:

- `bootstrapTools` install
- `binutils` built by `bootstrapTools`
- `gcc` built by `bootstrapTools`
- `glibc` built by `gcc`
- `gcc` built by `gcc`

As a result `glibc` gets built by fresher `gcc` and embeds a copy
of `libgcc_s.so.1`.

Co-authored-by: Rick van Schijndel <Mindavi@users.noreply.github.com>
@trofi trofi force-pushed the extra-gcc-build-during-bootstrap branch from 8ee2b7c to 03da08b Compare January 15, 2023 06:46
@trofi
Copy link
Contributor Author

trofi commented Jan 15, 2023

Rebased past gettext conflict.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ labels Jan 15, 2023
@wegank wegank mentioned this pull request Jan 16, 2023
13 tasks
@trofi trofi mentioned this pull request Jan 16, 2023
13 tasks
@wegank
Copy link
Member

wegank commented Jan 26, 2023

@ofborg build icu

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not merge this.

It causes an extra three recompiles of gcc all platforms (six instead of three), even platforms for which it gives no benefit.

#209870 provides all the same benefits, with no extra rebuilds, and lets you start using stdenv as soon as the second compile of gcc is finished (i.e. in parallel with the third compile).

@K900
Copy link
Contributor

K900 commented Jan 26, 2023

@trofi should we just close this as superseded maybe?

@trofi
Copy link
Contributor Author

trofi commented Jan 26, 2023

Sure. I did not yet get the feedback from @Ericson2314 and I think it's a prerequisite. If the PR languished for a month and problem persisted for as long as I used nixpkgs it can wait for another year.

You will need another form of the #208412

Not the first time I (and others) threw away most of the effort around the bootstrap bootstrap improvements away. I can keep the patch locally indefinitely. I already have about 200 of them :)

@trofi trofi closed this Jan 26, 2023
@trofi trofi deleted the extra-gcc-build-during-bootstrap branch January 26, 2023 10:38
@K900
Copy link
Contributor

K900 commented Jan 26, 2023

All I meant was that #209870 achieves the same thing and seems to be a better approach. Are there issues with that change that don't occur with this one?

@trofi
Copy link
Contributor Author

trofi commented Jan 26, 2023

Good question. I don't know. It's too hard for me to reason about it as a whole. I usually diff before/after state to see the difference. It's very hard to do there.

@ghost
Copy link

ghost commented Jan 26, 2023

I usually diff before/after state to see the difference. It's very hard to do there.

I initially tried factoring it into multiple PRs, but ended up getting "i don't see the point of this" feedback for the parts that were not useful separately from external-bootstrap (example). So to avoid time being wasted on those unproductive discussions I put it all under a single PR.

If you think about it, there's really no reason for people to merge isolated parts of #209870 since none of it is useful without the whole thing.

If you want to review it in pieces it isn't hard; the major changes are in only two files:

  • pkgs/stdenv/linux/default.nix
  • development/compilers/gcc/common/external-bootstrap.nix

Everything else is just fixing breakage (like emacs nativeComp) and threading around new parameters.

I could make the PR smaller if it were okay to break emacs, but that's not okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants